Fixed "Using Client Certificate for Elastic Search Authentication" #1139

clyang82 · 2018-10-25T06:41:32Z

Signed-off-by: Chun Lin Yang [email protected]

Which problem is this PR solving?

Resolves #678

Short description of the changes

Go driver support it with HTTPClient. so follow this way to support client certificate for ES

codecov · 2018-10-25T06:51:23Z

Codecov Report

Merging #1139 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1139   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         144     144           
  Lines        6779    6804   +25     
======================================
+ Hits         6779    6804   +25

Impacted Files	Coverage Δ
plugin/storage/es/options.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b22e43...1fe7a51. Read the comment docs.

jpkrohling

I have only a couple of minor comments, LGTM otherwise.

Would you mind opening an issue to add documentation to this new option?

jpkrohling · 2018-10-25T10:05:36Z

pkg/es/config/config.go

+}
+
+// LoadPrivateKeyFrom is used to load the private certificate and key for TLS
+func (c *Configuration) LoadPrivateKeyFrom() (*tls.Certificate, error) {


Perhaps remove the "From" from the function name?

Thanks for your comments. I will update

jpkrohling · 2018-10-25T10:06:51Z

pkg/es/config/config.go

+			return nil
+		}
+		httpClient := &http.Client{
+			Timeout: c.Timeout,


Seems like this is something extra, but I think it's ok to have it here.

This is only being applied when TLS is enabled. Can you make it available for every case?

@pavolloffay Yes. This is only being applied for httpclient which is being used by TLS case. I cannot find where to add for every case. we use BulkProcessorService which allows to easily process bulk requests. There is no interface to set timeout. Could you point me out more specifically? Thanks.

So the driver does not use the client when tls is disabled?

Something like this would not work?

// GetConfigs wraps the configs to feed to the ElasticSearch client init func (c *Configuration) GetConfigs(logger *zap.Logger) []elastic.ClientOptionFunc { options := []elastic.ClientOptionFunc{elastic.SetURL(c.Servers...), elastic.SetSniff(c.Sniffer)} httpClient := &http.Client{ Timeout: c.Timeout, } if c.TLS.Enabled { ctlsConfig, err := c.TLS.createTLSConfig(logger) if err != nil { return nil } httpClient.Transport = &http.Transport{ TLSClientConfig:ctlsConfig, } } else { options = append(options, elastic.SetBasicAuth(c.Username, c.Password)) } options = append(options, elastic.SetHttpClient(httpClient)) return options }

for elastic v5, if no HttpClient is configured, then http.DefaultClient is used. so your suggestion should be working. I will have a test later. Thanks.

yurishkuro

Thanks for contributing.

Looks awesome overall! I only have minor nits.

Could you explain how you tested this, maybe post some evidence / examples.

pkg/es/config/config.go

yurishkuro · 2018-10-25T19:30:06Z

pkg/es/config/config.go

 	options[0] = elastic.SetURL(c.Servers...)
 	options[1] = elastic.SetBasicAuth(c.Username, c.Password)
 	options[2] = elastic.SetSniff(c.Sniffer)
 	return options
 }
+
+// CreateTLSConfig creates TLS Configuration to connect with ES Cluster.
+func (c *Configuration) CreateTLSConfig(logger *zap.Logger) (*tls.Config, error) {


I think all these new functions can be private. Also, can they be on the TLS config object instead of the top-level Configuration type?

Yes. Agree with your point. I will make changes accordingly.

pkg/es/config/config.go

yurishkuro · 2018-10-25T19:43:06Z

Does this address the same issue as #1036 ?

clyang82 · 2018-10-26T04:21:44Z

@jpkrohling Document issue logged - #1144
Will update the document later.

clyang82 · 2018-10-26T04:33:23Z

@yurishkuro it is similar with #1036. both need pass httpclient to elasticsearch client. but AWS Identity and Access Management (IAM) is a web service that helps you securely control access to AWS resources. that need to call AWS api to do authentication, that means need to pass a aws http client instance to elasticsearch client.

clyang82 · 2018-10-29T01:35:31Z

@jpkrohling & @yurishkuro Could you please help to merge my pull request if you have no further comments? Thanks.

pavolloffay

I think the timeout should be configurable for all cases.

pavolloffay · 2018-10-29T10:25:56Z

pkg/es/config/config.go

+			return nil
+		}
+		httpClient := &http.Client{
+			Timeout: c.Timeout,


This is only being applied when TLS is enabled. Can you make it available for every case?

pavolloffay · 2018-10-30T12:34:39Z

@clyang82 do you want to fix it in this PR or in a follow-up PR?

yurishkuro · 2018-10-30T14:39:41Z

pkg/es/config/config.go

-	options[2] = elastic.SetSniff(c.Sniffer)
+func (c *Configuration) GetConfigs(logger *zap.Logger) []elastic.ClientOptionFunc {
+	options := make([]elastic.ClientOptionFunc, 0)
+	options = append(options, elastic.SetURL(c.Servers...), elastic.SetSniff(c.Sniffer))


can combine

options := []elastic.ClientOptionFunc{ elastic.SetURL(c.Servers...), elastic.SetSniff(c.Sniffer) }

yurishkuro · 2018-10-30T14:42:48Z

pkg/es/config/config.go

+	if err != nil {
+		logger.Fatal("Couldn't load root certificate", zap.Error(err))
+	}
+	if len(tlsConfig.CertPath) > 0 && len(tlsConfig.KeyPath) > 0 {


why is this check needed? If it fails you return nil, so shouldn't it be an error to set c.TLS.Enabled and not provide the other 3 params?

yurishkuro · 2018-10-30T14:43:44Z

pkg/es/config/config.go

+}
+
+// loadPrivateKeyFrom is used to load the private certificate and key for TLS
+func (tlsConfig *TLSConfig) loadPrivateKeyFrom() (*tls.Certificate, error) {


nit: remove "From" suffix

yurishkuro · 2018-10-30T14:47:39Z

it is similar with #1036. both need pass httpclient to elasticsearch client. but AWS Identity and Access Management (IAM) is a web service that helps you securely control access to AWS resources. that need to call AWS api to do authentication, that means need to pass a aws http client instance to elasticsearch client.

Is it necessary to use IAM with AWS ES? Can your approach be used and have some other script retrieve the credentials from IAM? If we can use your approach then I think it's better than #1036 since it does not introduce additional dependencies aside from std lib.

wesleyk · 2018-10-30T15:01:13Z

it is similar with #1036. both need pass httpclient to elasticsearch client. but AWS Identity and Access Management (IAM) is a web service that helps you securely control access to AWS resources. that need to call AWS api to do authentication, that means need to pass a aws http client instance to elasticsearch client.

Is it necessary to use IAM with AWS ES? Can your approach be used and have some other script retrieve the credentials from IAM? If we can use your approach then I think it's better than #1036 since it does not introduce additional dependencies aside from std lib.

These changes are a bit orthogonal from using IAM credentials IMO, as in the IAM case you need to provide a custom http client.

That being said, the client cert approach for authentication may be a reasonable tradeoff over IAM. Looking more into that now.

yurishkuro · 2018-10-31T18:48:00Z

Relevant blog post: http://labs.vistarmedia.com/2018/10/31/deploying-jaeger-with-cloudformation-via-bazel.html

pavolloffay · 2018-11-01T12:44:08Z

@jpkrohling We could link the blog post in the newsletter.

clyang82 · 2018-11-02T06:09:48Z

@pavolloffay the latest code changes have passed my testing. Thanks.

pavolloffay · 2018-11-02T08:50:49Z

If this approach is orthogonal to IAM we could go forward and merge.

pavolloffay

LGTM, the fatal logging looks a bit weird. We usually log at error level and propagate the errors.

yurishkuro · 2018-11-02T17:01:29Z

pkg/es/config/config.go

@@ -64,7 +78,7 @@ func (c *Configuration) NewClient(logger *zap.Logger, metricsFactory metrics.Fac
 	if len(c.Servers) < 1 {
 		return nil, errors.New("No servers specified")
 	}
-	rawClient, err := elastic.NewClient(c.GetConfigs()...)
+	rawClient, err := elastic.NewClient(c.GetConfigs(logger)...)


can we do this without logging?

options, err := c.getConfigOptions() // note name change, no logger if err != nil { return nil, err } rawClient, err := elastic.NewClient(options...)

yurishkuro · 2018-11-02T17:02:20Z

pkg/es/config/config.go

+	return &tls.Config{
+		RootCAs:      rootCerts,
+		Certificates: []tls.Certificate{*clientPrivateKey},
+	}, err


Signed-off-by: Chun Lin Yang <[email protected]>

clyang82 · 2018-11-08T02:53:32Z

@jpkrohling & @yurishkuro Could you please help to merge my pull request? Thanks.

jpkrohling

This LGTM and @pavolloffay has approved as well. As @yurishkuro had some comments before, I'll wait for a review from him before merging.

yurishkuro · 2018-11-09T14:36:32Z

plugin/storage/es/options.go

@@ -119,6 +124,10 @@ func addFlags(flagSet *flag.FlagSet, nsConfig *namespaceConfig) {
 		nsConfig.namespace+suffixServerURLs,
 		nsConfig.servers,
 		"The comma-separated list of ElasticSearch servers, must be full url i.e. http://localhost:9200")
+	flagSet.Duration(
+		nsConfig.namespace+suffixTimeout,
+		nsConfig.Timeout,


I don't see where a default value for this is set. I assume it's not becoming a required flag, correct? Maybe expand in the help string what happens if not set.

@yurishkuro Yes. It is not a required flag. the default value is 0. A Timeout of zero means no timeout. so how about change the help string from Timeout used for queries to Timeout used for queries. A Timeout of zero means no timeout.

new PR for this fix - #1171

yurishkuro · 2018-11-09T14:37:57Z

Looks good to me, thanks! Had just one nit, but it can be done in another PR.

clyang82 requested review from black-adder, jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners October 25, 2018 06:41

clyang82 mentioned this pull request Oct 25, 2018

Deploy individual jaeger components(agent/collector/query) instead of all-in-one istio/istio#8965

Closed

jpkrohling approved these changes Oct 25, 2018

View reviewed changes

yurishkuro reviewed Oct 25, 2018

View reviewed changes

clyang82 force-pushed the es-client-certificate branch from 7ef1a58 to d01b5df Compare October 26, 2018 03:32

clyang82 force-pushed the es-client-certificate branch from d01b5df to e981b19 Compare October 29, 2018 01:12

pavolloffay requested changes Oct 29, 2018

View reviewed changes

yurishkuro reviewed Oct 30, 2018

View reviewed changes

yurishkuro mentioned this pull request Oct 30, 2018

Enable IAM based auth to ES for AWS clients (#465) #1036

Closed

clyang82 force-pushed the es-client-certificate branch from e981b19 to 061c460 Compare October 31, 2018 05:09

clyang82 requested a review from tiffon as a code owner October 31, 2018 05:09

clyang82 force-pushed the es-client-certificate branch from 0e7605f to 51919a4 Compare November 2, 2018 01:15

clyang82 requested a review from objectiser as a code owner November 2, 2018 01:15

pavolloffay approved these changes Nov 2, 2018

View reviewed changes

yurishkuro reviewed Nov 2, 2018

View reviewed changes

clyang82 added 6 commits November 5, 2018 12:44

Fixed jaegertracing#678

1786537

Signed-off-by: Chun Lin Yang <[email protected]>

Fixed jaegertracing#678

cb7fc0c

Signed-off-by: Chun Lin Yang <[email protected]>

address review comments

cb489f0

Signed-off-by: Chun Lin Yang <[email protected]>

address review comments

e299315

Signed-off-by: Chun Lin Yang <[email protected]>

support timeout for non-tls-enabled case

b915241

Signed-off-by: Chun Lin Yang <[email protected]>

change method name

9b0050e

Signed-off-by: Chun Lin Yang <[email protected]>

clyang82 force-pushed the es-client-certificate branch from 8db0cce to 9b0050e Compare November 5, 2018 04:44

clyang82 mentioned this pull request Nov 5, 2018

WIP: tracing: Change tracing helm chart to use jaeger-operator istio/istio#9508

Closed

jpkrohling approved these changes Nov 8, 2018

View reviewed changes

yurishkuro approved these changes Nov 9, 2018

View reviewed changes

Merge branch 'master' into es-client-certificate

1fe7a51

ghost assigned yurishkuro Nov 9, 2018

ghost added the review label Nov 9, 2018

yurishkuro merged commit de8a9ad into jaegertracing:master Nov 9, 2018

ghost removed the review label Nov 9, 2018

clyang82 mentioned this pull request Nov 13, 2018

Provide more information for timeout #1171

Merged

clyang82 mentioned this pull request Nov 26, 2018

REQUEST: New membership for clyang82 istio/community#102

Closed

7 tasks

jpkrohling mentioned this pull request Apr 2, 2019

Support AWS Elasticsearch IAM authentication by signing requests #465

Closed

samudurand mentioned this pull request Oct 15, 2019

Documentation for using AWS Elasticsearch #1863

Closed

Fixed "Using Client Certificate for Elastic Search Authentication" #1139

Fixed "Using Client Certificate for Elastic Search Authentication" #1139

Conversation

clyang82 commented Oct 25, 2018 • edited by pavolloffay Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Oct 25, 2018 • edited Loading

Codecov Report

jpkrohling left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Oct 25, 2018

clyang82 commented Oct 26, 2018

clyang82 commented Oct 26, 2018

clyang82 commented Oct 29, 2018

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Oct 30, 2018

wesleyk commented Oct 30, 2018

yurishkuro commented Oct 31, 2018

pavolloffay commented Nov 1, 2018

clyang82 commented Nov 2, 2018

pavolloffay commented Nov 2, 2018

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clyang82 commented Nov 8, 2018

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Nov 9, 2018

clyang82 commented Oct 25, 2018 •

edited by pavolloffay

Loading

codecov bot commented Oct 25, 2018 •

edited

Loading

jpkrohling left a comment •

edited

Loading

pavolloffay commented Oct 30, 2018 •

edited

Loading